Fix connect ECONNREFUSED error when trying to debug on iOS Sim #2704
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When trying to debug on iOS Simulator (via NativeScript Inspector), we often receive `Error: connect ECONNREFUSED 127.0.0.1:18181` error. There are two different problems. First one is incorrect identifying of the application's PID - we are using `_.trimStart` method totally incorrectly and in case your application identifier has numbers in it, they'll also be replaced in the search PID string. For example in case your app id is `org.nativescript.app10`, and the real PID of the running application is 18129, the current usage of `trimStart` method will give you the PID 8129. Fix this by applying regular expression and find the correct PID. The second problem is that there's some delay between opening the NativeScript Inspector and the debugged application on the device. In many cases the first time when we try to connect, we receive the error ECONNREFUSED. However in the previous implementation, we were trying to connect multiple times. Get back the old code and adjust it to work with Promises instead of the old sync execution.
yyosifov
reviewed
Apr 10, 2017
| }); | ||
|
|
||
| frontendSocket.on("close", () => { | ||
| console.log("frontend socket closed"); |
There was a problem hiding this comment.
shouldn't this be $logger
yyosifov
reviewed
Apr 10, 2017
| } | ||
| }); | ||
| backendSocket.on("close", () => { | ||
| console.log("backend socket closed"); |
There was a problem hiding this comment.
shouldn't this be $logger
yyosifov
reviewed
Apr 10, 2017
lib/services/ios-debug-service.ts
Outdated
| @@ -103,11 +103,18 @@ class IOSDebugService extends DebugServiceBase implements IPlatformDebugService | |||
|
|
|||
| let lineStream = byline(child_process.stdout); | |||
| this._childProcess = child_process; | |||
| const pidRegExp = new RegExp(`${debugData.applicationIdentifier}:\\s?(\\d+)`); | |||
There was a problem hiding this comment.
Would be nice to have Unit Tests for this Regular Expression. I'd suggest we extract this to some method/service so that we can test it and cover with more expressions?
There was a problem hiding this comment.
I'm not sure where's the appropriate place for this code. I agree that it needs tests and I'll add them in a separate PR.
There was a problem hiding this comment.
Nevermind - I've created a new helper method and added unit tests for it: telerik/mobile-cli-lib#942
yyosifov
reviewed
Apr 10, 2017
lib/services/ios-debug-service.ts
Outdated
| const pidMatch = lineText.match(pidRegExp); | ||
|
|
||
| if (!pidMatch) { | ||
| this.$logger.trace(`Line ${lineText} does not contain the searched pattern: ${pidRegExp}.`); |
There was a problem hiding this comment.
if there's no pidMatch, what happens with const pid = pidMatch[1] below? Won't an undefined exception occur?
There was a problem hiding this comment.
I've forgotten to commit a return here 😄 Thanks for pointing it.
rosen-vladimirov
force-pushed
the
vladimirov/fix-econnreset-emulator
branch
from
16f506b to
7ab62ce
Compare
lib/commands/debug.ts
Outdated
|
|
||
| protected printDebugInformation(information: string[]): void { | ||
| if (this.$options.chrome) { | ||
| _.each(information, i => { |
There was a problem hiding this comment.
call super.printDebugInformation here instead of the each
TsvetanMilanov
approved these changes
Apr 10, 2017
- `tns debug ios` prints message that you have to open `null` url in Chrome - when you do not pass `--chrome` to this command, it will use NativeScript inspector, so hide the incorrect message. - debugData is constructed too early in the command - in case the application has not been built before calling `tns debug ...`, construction of debugData will fail as CLI will not be able to find the latest built package. In order to fix this make the `pathToAppPackage` not mandatory (we do not need it for debug commands when `--start` is passed) and populate it after successful deploy of the application. This way it will have correct value. Delete most of the DebugDataService as most of the methods are not needed with these changes. - remove the check if `--chrome` is passed in order to print the url when `tns debug android` is used. The check was incorrect (it should check the value of `options.client`), but in fact we do not need it - the idea of the check was to suppress starting of Node Inspector, however we do not start it anymore no matter of the passed flags. So remove the incorrect check.
rosen-vladimirov
force-pushed
the
vladimirov/fix-econnreset-emulator
branch
from
7ab62ce to
8180c70
Compare
|
ping @yyosifov - all comments are fixed |
Move the logic for getting PID of application started on iOS Simulator to mobile-cli-lib. Add new helper method to get the process ID of an application from iOS Simulator Logs. Whenever we start an application on iOS Simulator, in its logs we can find the PID of the process in the following format: ``` <app id>: <app id>: <PID> ``` Add unit tests for the method.
rosen-vladimirov
force-pushed
the
vladimirov/fix-econnreset-emulator
branch
from
72619c0 to
df7b887
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When trying to debug on iOS Simulator (via NativeScript Inspector), we often receive
Error: connect ECONNREFUSED 127.0.0.1:18181error.There are two different problems. First one is incorrect identifying of the application's PID - we are using
_.trimStartmethod totally incorrectly and in case your application identifier has numbers in it, they'll also be replaced in the search PID string. For example in case your app id isorg.nativescript.app10, and the real PID of the running application is 18129, the current usage oftrimStartmethod will give you the PID 8129.Fix this by applying regular expression and find the correct PID.
The second problem is that there's some delay between opening the NativeScript Inspector and the debugged application on the device. In many cases the first time when we try to connect, we receive the error ECONNREFUSED. However in the previous implementation, we were trying to connect multiple times. Get back the old code and adjust it to work with Promises instead of the old sync execution.
Second commit:
Fix debug command:
tns debug iosprints message that you have to opennullurl in Chrome - when you do not pass--chrometo this command, it will use NativeScript inspector, so hide the incorrect message.tns debug ..., construction of debugData will fail as CLI will not be able to find the latest built package. In order to fix this make thepathToAppPackagenot mandatory (we do not need it for debug commands when--startis passed) and populate it after successful deploy of the application. This way it will have correct value. Delete most of the DebugDataService as most of the methods are not needed with these changes.--chromeis passed in order to print the url whentns debug androidis used. The check was incorrect (it should check the value ofoptions.client), but in fact we do not need it - the idea of the check was to suppress starting of Node Inspector, however we do not start it anymore no matter of the passed flags. So remove the incorrect check.